fix: exit stdio server cleanly on interrupt - #2745
Conversation
This comment was marked as spam.
This comment was marked as spam.
|
Updated the regression test to match the transport-wide scope. Changes:
Validation run locally:
|
|
Pushed a follow-up for the CI Root cause: after the test was parametrized over Change:
Validation run locally:
I also tried to run |
958ecbe to
8da5386
Compare
|
One more CI-specific coverage follow-up is now included in the same amended commit. The next Linux run showed all tests passing, but branch coverage dropped to 99.99% because coverage.py reported a missing Validation re-run locally:
|
|
The two remaining 3.14 lowest-direct jobs failed outside the I pushed a small follow-up commit that keeps the clean-exit assertion but allows dependency warnings to appear before the marker. It also adds a local Local validation: |
4655d30 to
9d42000
Compare
|
Rebased this branch on current The only conflict was in the stdio subprocess stderr assertion after upstream changed the same test. I kept the newer main-side coverage/env/timeout handling, kept the Validated locally:
|
ErenAta16
left a comment
There was a problem hiding this comment.
Verified the fix does what it says and the diagnosis in the issue is accurate: run() calls anyio.run(...) with no guard, so Ctrl-C during the stdio receive loop surfaces the whole anyio/asyncio cancellation chain.
Heads up that this is one of three open PRs with a byte-identical source change: yours, #2778, and #3027 all wrap the match transport: block in try/except KeyboardInterrupt: return, same 10 added / 7 removed lines in the same place. Yours has priority by date (Jun 1). I've put the full comparison on #2663 so a maintainer doesn't have to derive it three times.
What distinguishes yours is the test: parametrising over all three transports and asserting run.assert_called_once_with(mcp.run_stdio_async) is stronger than the single-transport version the other two have, since it pins that the guard wraps every branch rather than just stdio.
Two things I'd tidy before it lands.
Exit code. except KeyboardInterrupt: return makes run() return normally, so the process exits 0. Measured both shapes:
except KeyboardInterrupt: return -> exit code 0
raise SystemExit(130) -> exit code 130
130 is 128 + SIGINT, which is what an uncaught KeyboardInterrupt produces today and what a shell or supervisor expects from a Ctrl-C. After this change a script doing mcp-server || handle_failure reads an interrupted run as a clean success.
If the goal is only to drop the traceback, raise SystemExit(130) gets that without changing the signal semantics. If exiting 0 is deliberate (say, because a supervisor is meant to treat Ctrl-C as a normal stop) that's a defensible choice, it just deserves a comment, otherwise someone will "fix" it later assuming it was an oversight.
Unrelated hunks. The cast(TextIO, errlog) change in tests/interaction/transports/test_stdio.py is a typing fix that has nothing to do with this bug, and the # pragma: no cover -> # pragma: no branch edits are a coverage side effect of parametrising. Both are reasonable in themselves, but they make the diff larger than "guard the interrupt" and give a reviewer more to weigh. Splitting them out would make this the easiest of the three to approve.
Summary
KeyboardInterruptat the synchronousMCPServer.run()boundaryTo verify
.\.venv\Scripts\python.exe -m pytest tests\server\mcpserver\test_server.py::TestServer::test_stdio_keyboard_interrupt_exits_cleanly -q.\.venv\Scripts\python.exe -m ruff check src\mcp\server\mcpserver\server.py tests\server\mcpserver\test_server.py.\.venv\Scripts\python.exe -m ruff format --check src\mcp\server\mcpserver\server.py tests\server\mcpserver\test_server.py.\.venv\Scripts\pyright.exe src\mcp\server\mcpserver\server.py tests\server\mcpserver\test_server.pygit diff --checkRefs #2663